-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: fe/config du monitor #1752
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto Pull Request Review from LlamaPReview
Large PR Notification
Dear contributor,
Thank you for your substantial contribution to this project. LlamaPReview has detected that this Pull Request contains a large volume of changes, which exceeds our current processing capacity.
Details:
- PR and related contents total size: Approximately 52,975 characters
- Current limit: 50,000 characters
Next steps:
- Consider breaking this PR into smaller, more focused changes if possible.
- For manual review, please reach out to your team members or maintainers.
We appreciate your understanding and commitment to improving this project. Your contributions are valuable, and we want to ensure they receive the attention they deserve.
LlamaPReview is continuously evolving to better serve the community. Share your thoughts on handling large PRs in our GitHub Discussions - your feedback helps us improve and expand our capabilities.
If you have any questions or need assistance, our community and support team are here to help.
Best regards,
LlamaPReview Team
WalkthroughThis pull request implements distributed uptime monitor enhancements across both client and server components. On the client side, new custom hooks have been added for creating, updating, fetching, and deleting monitors, and relevant UI components and routing have been updated accordingly. Additionally, property names and conditional checks have been adjusted for consistency, and network service methods have been refactored to use destructuring. On the server side, the deletion logic has been restructured with concurrent operations and enhanced error handling for monitors, distributed checks, and status pages. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as CreateDistributedUptime Component
participant H as useCreateDistributedUptimeMonitor Hook
participant NS as NetworkService
U->>C: Submit monitor form
C->>H: Invoke create/update function
H->>NS: Call createMonitor/updateMonitor API
NS-->>H: Return response
H->>C: Update loading and error states
C->>U: Render updated monitor data
sequenceDiagram
participant U as User
participant D as DistributedUptimeDetails Component
participant CH as ControlsHeader
participant H as useDeleteMonitor Hook
participant NS as NetworkService
U->>CH: Click delete button
CH->>D: Open confirmation dialog
U->>D: Confirm deletion
D->>H: Trigger deleteMonitor function
H->>NS: Call deleteMonitorById API
NS-->>H: Return deletion response
H->>D: Update deletion state
D->>U: Navigate to monitor list
Possibly related PRs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🔭 Outside diff range comments (4)
Client/src/Pages/DistributedUptime/Monitors/Components/MonitorTable/index.jsx (1)
65-65
:⚠️ Potential issueMom's spaghetti alert: Missing actions implementation!
The TODO placeholder needs to be replaced with actual actions for editing and deleting DU monitors, as mentioned in the PR objectives.
Consider implementing an ActionsMenu component similar to the one used in UptimeDataTable:
-render: (row) => <span>{"TODO"}</span>, +render: (row) => ( + <ActionsMenu + monitor={row.monitor} + isAdmin={isAdmin} + updateRowCallback={triggerUpdate} + pauseCallback={triggerUpdate} + /> +),Client/src/Pages/DistributedUptime/Create/index.jsx (1)
177-177
:⚠️ Potential issueRemove console.log and handle form submission properly.
The form's onSubmit handler contains a console.log and doesn't prevent default form submission:
- onSubmit={() => console.log("submit")} + onSubmit={(e) => { + e.preventDefault(); + handleCreateMonitor(); + }}Client/src/Features/UptimeMonitors/uptimeMonitorsSlice.js (1)
86-91
: 🛠️ Refactor suggestionRemove unused
updatedFields
object.The
updatedFields
object is defined but never used in the API call, as the entiremonitor
object is passed instead. This creates dead code.Apply this diff to remove the unused code:
- const updatedFields = { - name: monitor.name, - description: monitor.description, - interval: monitor.interval, - notifications: monitor.notifications, - };Also applies to: 95-95
Client/src/Utils/NetworkService.js (1)
429-434
: 🛠️ Refactor suggestionUse optional chaining for notifications mapping.
The current implementation could throw if notifications is undefined.
Apply this diff to use optional chaining:
- notifications && - notifications.map(async (notification) => { + notifications?.map?.(async (notification) => {
🧹 Nitpick comments (9)
Client/src/Pages/Uptime/Monitors/Components/UptimeDataTable/index.jsx (1)
25-36
: Knees weak, arms heavy: JSDoc needs updating!The JSDoc documentation still references the
title
property in the monitors array type definition, but the code now usesname
. Let's update the documentation to match the implementation.* @param {Array<{ * _id: string, * url: string, -* title: string, +* name: string, * percentage: number, * percentageColor: string, * monitor: {Client/src/Pages/DistributedUptime/Details/Hooks/useDeleteMonitor.jsx (1)
6-25
: Yo dawg, add cleanup and success feedback!The hook's got some missing pieces that could make it more robust:
- Add cleanup to prevent state updates after unmount
- Add success toast notification for better UX
Here's how to level up this code:
const useDeleteMonitor = ({ monitorId }) => { const [isLoading, setIsLoading] = useState(false); const { authToken } = useSelector((state) => state.auth); + const mounted = useRef(true); + + useEffect(() => { + return () => { + mounted.current = false; + }; + }, []); + const deleteMonitor = async () => { try { setIsLoading(true); await networkService.deleteMonitorById({ authToken, monitorId }); + if (mounted.current) { + createToast({ + body: "Monitor deleted successfully", + type: "success" + }); + } return true; } catch (error) { - createToast({ - body: error.message, - }); + if (mounted.current) { + createToast({ + body: error.message, + type: "error" + }); + } return false; } finally { - setIsLoading(false); + if (mounted.current) { + setIsLoading(false); + } } };Client/src/Pages/DistributedUptime/Create/Hooks/useCreateDistributedUptimeMonitor.jsx (1)
6-31
: Arms are heavy from carrying unused variables!The hook needs some cleanup and improvements:
- Remove unused user variable
- Add form validation
- Consider optimistic updates
Here's how to clean it up:
const useCreateDistributedUptimeMonitor = ({ isCreate, monitorId }) => { - const { authToken, user } = useSelector((state) => state.auth); + const { authToken } = useSelector((state) => state.auth); const [isLoading, setIsLoading] = useState(false); const [networkError, setNetworkError] = useState(false); + + const validateForm = (form) => { + if (!form.name?.trim()) { + throw new Error('Monitor name is required'); + } + // Add more validation as needed + }; + const createDistributedUptimeMonitor = async ({ form }) => { setIsLoading(true); try { + validateForm(form); + if (isCreate) { await networkService.createMonitor({ authToken, monitor: form }); } else { await networkService.updateMonitor({ authToken, monitor: form, monitorId }); } + createToast({ + body: `Monitor ${isCreate ? 'created' : 'updated'} successfully`, + type: 'success' + }); return true; } catch (error) { setNetworkError(true);Server/db/mongo/modules/statusPageModule.js (1)
193-201
: Yo, let's get some feedback on those deletions! 💪The function should return the deletion result to help track how many documents were affected.
Here's how to improve it:
const deleteStatusPagesByMonitorId = async (monitorId) => { try { - await StatusPage.deleteMany({ monitors: { $in: [monitorId] } }); + const result = await StatusPage.deleteMany({ monitors: { $in: [monitorId] } }); + return result; } catch (error) { error.service = SERVICE_NAME; error.method = "deleteStatusPageByMonitorId"; throw error; } };This way, callers can verify how many status pages were actually deleted.
Client/src/Pages/DistributedUptime/Create/index.jsx (4)
34-40
: Add input validation to parseUrl utility.While the try-catch handles parsing errors, consider adding input validation:
const parseUrl = (url) => { + if (!url || typeof url !== 'string') { + return null; + } try { return new URL(url); } catch (error) { return null; } };
57-62
: Add interval validation in form state.Consider adding validation to ensure interval stays within SELECT_VALUES range:
const [form, setForm] = useState({ type: "distributed_http", name: "", url: "", - interval: 1, + interval: Math.max(1, Math.min(SELECT_VALUES[SELECT_VALUES.length - 1]._id, 1)), });
78-93
: Add cleanup to useEffect.Consider adding a cleanup function to handle component unmounting:
useEffect(() => { + let mounted = true; if (typeof monitor !== "undefined") { const parsedUrl = parseUrl(monitor?.url); const protocol = parsedUrl?.protocol?.replace(":", "") || ""; - setHttps(protocol === "https"); + if (mounted) { + setHttps(protocol === "https"); + } const newForm = { name: monitor.name, interval: monitor.interval / MS_PER_MINUTE, url: parsedUrl.host, type: monitor.type, }; - setForm(newForm); + if (mounted) { + setForm(newForm); + } } + return () => { + mounted = false; + }; }, [monitor]);
96-129
: Enhance error messaging in handleCreateMonitor.Consider providing more specific error messages:
- createToast({ body: "Failed to create monitor." }); + createToast({ + body: isCreate + ? "Failed to create monitor. Please try again or contact support." + : "Failed to update monitor. Please try again or contact support." + });Server/db/mongo/modules/monitorModule.js (1)
611-620
: Enhance filter match stage readability.The nested match stage could be flattened for better readability.
Consider this alternative structure:
- { - $match: { - $or: [ - { name: { $regex: filter, $options: "i" } }, - { url: { $regex: filter, $options: "i" } }, - ], - }, - }, + { + $match: { + $or: [ + { name: { $regex: filter, $options: "i" } }, + { url: { $regex: filter, $options: "i" } }, + ], + }, + },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Client/src/Features/UptimeMonitors/uptimeMonitorsSlice.js
(1 hunks)Client/src/Pages/DistributedUptime/Create/Hooks/useCreateDistributedUptimeMonitor.jsx
(1 hunks)Client/src/Pages/DistributedUptime/Create/Hooks/useMonitorFetch.jsx
(1 hunks)Client/src/Pages/DistributedUptime/Create/index.jsx
(10 hunks)Client/src/Pages/DistributedUptime/Details/Components/ControlsHeader/index.jsx
(1 hunks)Client/src/Pages/DistributedUptime/Details/Hooks/useDeleteMonitor.jsx
(1 hunks)Client/src/Pages/DistributedUptime/Details/index.jsx
(3 hunks)Client/src/Pages/DistributedUptime/Monitors/Components/MonitorTable/index.jsx
(1 hunks)Client/src/Pages/DistributedUptime/Monitors/index.jsx
(1 hunks)Client/src/Pages/DistributedUptimeStatus/Create/index.jsx
(1 hunks)Client/src/Pages/StatusPage/Create/index.jsx
(1 hunks)Client/src/Pages/Uptime/Monitors/Components/UptimeDataTable/index.jsx
(1 hunks)Client/src/Routes/index.jsx
(1 hunks)Client/src/Utils/NetworkService.js
(2 hunks)Server/controllers/monitorController.js
(2 hunks)Server/db/mongo/modules/distributedCheckModule.js
(1 hunks)Server/db/mongo/modules/monitorModule.js
(3 hunks)Server/db/mongo/modules/statusPageModule.js
(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
Server/controllers/monitorController.js
[error] 429-433: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
Client/src/Pages/DistributedUptime/Monitors/Components/MonitorTable/index.jsx (1)
32-32
: Yo dawg, we need to verify this property name change!The change from
row.title
torow.name
looks like part of standardizing property names across monitor components. However, we should verify that all data sources provide thename
property to avoid any undefined values showing up in production.Let's run this script to check for any other occurrences of
row.title
that might need updating:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining instances of row.title rg "row\.title" --type js --type jsxLength of output: 66
Here's an updated approach to verify that the change from
row.title
torow.name
has been applied throughout the codebase, including JSX files:If this script returns no output, it indicates that all occurrences of
row.title
have been replaced withrow.name
. This helps ensure that the MonitorTable component now consistently usesrow.name
without risking undefined values in production.Let me know the result, eh!
🏁 Script executed:
#!/bin/bash # Correct the search to include .jsx files by explicitly adding the type. rg --type-add "jsx:*.jsx" --type jsx "row\.title" .Length of output: 53
Property Change Fully Verified in MonitorTable Component
After running the updated search command that included
.jsx
files, no instances ofrow.title
were found anywhere in the codebase. This indicates that the change to userow.name
is applied consistently in the MonitorTable component.
- The source file at
Client/src/Pages/DistributedUptime/Monitors/Components/MonitorTable/index.jsx
now solely usesrow.name
.- No legacy references to
row.title
remain in JavaScript/JSX files.Everything looks solid, eh?
Client/src/Pages/DistributedUptime/Monitors/index.jsx (1)
44-44
: Yo, this condition improvement is fire! 🔥The updated condition now properly handles both undefined and empty states, making the code more robust.
Client/src/Pages/StatusPage/Create/index.jsx (1)
49-49
: LGTM! Clean and simple isCreate check.The simplified logic for determining creation mode is more maintainable and reduces dependencies.
Client/src/Pages/DistributedUptimeStatus/Create/index.jsx (1)
25-25
: LGTM! Consistent isCreate implementation.The implementation matches the pattern used in StatusPage/Create, maintaining codebase consistency.
Client/src/Routes/index.jsx (1)
110-117
: LGTM! Well-structured route addition.The new route for distributed uptime monitor configuration:
- Follows the established routing pattern
- Properly implements route protection
- Uses consistent component naming
Server/controllers/monitorController.js (1)
300-340
: Well-structured concurrent operations with robust error handling!The refactoring improves the code by:
- Using Promise.allSettled for concurrent operations
- Providing detailed error logging for each failed operation
- Maintaining operation even if some deletions fail
Server/db/mongo/modules/monitorModule.js (1)
375-378
: Improved error handling with optional chaining!The use of optional chaining and nullish coalescing improves robustness by gracefully handling undefined values.
Client/src/Utils/NetworkService.js (1)
102-104
: Clean parameter destructuring!The destructuring of config parameters improves code readability.
useEffect(() => { | ||
const fetchMonitors = async () => { | ||
try { | ||
if (isCreate) return; | ||
const res = await networkService.getUptimeDetailsById({ | ||
authToken: authToken, | ||
monitorId: monitorId, | ||
normalize: true, | ||
}); | ||
setMonitor(res?.data?.data ?? {}); | ||
} catch (error) { | ||
setNetworkError(true); | ||
createToast({ body: error.message }); | ||
} finally { | ||
setIsLoading(false); | ||
} | ||
}; | ||
fetchMonitors(); | ||
}, [authToken, monitorId, isCreate]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Knees weak from missing cleanup and retries!
The fetch logic needs some improvements:
- Add abort controller for cleanup
- Add retry mechanism for network resilience
- Consider adding request caching
Here's how to make it stronger:
useEffect(() => {
+ const controller = new AbortController();
+ let retryCount = 0;
+ const MAX_RETRIES = 3;
+
const fetchMonitors = async () => {
try {
if (isCreate) return;
const res = await networkService.getUptimeDetailsById({
authToken: authToken,
monitorId: monitorId,
normalize: true,
+ signal: controller.signal
});
setMonitor(res?.data?.data ?? {});
} catch (error) {
+ if (error.name === 'AbortError') return;
+
+ if (retryCount < MAX_RETRIES) {
+ retryCount++;
+ console.log(`Retrying fetch (${retryCount}/${MAX_RETRIES})...`);
+ await new Promise(resolve => setTimeout(resolve, 1000 * retryCount));
+ return fetchMonitors();
+ }
+
setNetworkError(true);
createToast({ body: error.message });
} finally {
setIsLoading(false);
}
};
fetchMonitors();
+
+ return () => controller.abort();
}, [authToken, monitorId, isCreate]);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
useEffect(() => { | |
const fetchMonitors = async () => { | |
try { | |
if (isCreate) return; | |
const res = await networkService.getUptimeDetailsById({ | |
authToken: authToken, | |
monitorId: monitorId, | |
normalize: true, | |
}); | |
setMonitor(res?.data?.data ?? {}); | |
} catch (error) { | |
setNetworkError(true); | |
createToast({ body: error.message }); | |
} finally { | |
setIsLoading(false); | |
} | |
}; | |
fetchMonitors(); | |
}, [authToken, monitorId, isCreate]); | |
useEffect(() => { | |
const controller = new AbortController(); | |
let retryCount = 0; | |
const MAX_RETRIES = 3; | |
const fetchMonitors = async () => { | |
try { | |
if (isCreate) return; | |
const res = await networkService.getUptimeDetailsById({ | |
authToken: authToken, | |
monitorId: monitorId, | |
normalize: true, | |
signal: controller.signal | |
}); | |
setMonitor(res?.data?.data ?? {}); | |
} catch (error) { | |
if (error.name === 'AbortError') return; | |
if (retryCount < MAX_RETRIES) { | |
retryCount++; | |
console.log(`Retrying fetch (${retryCount}/${MAX_RETRIES})...`); | |
await new Promise(resolve => setTimeout(resolve, 1000 * retryCount)); | |
return fetchMonitors(); | |
} | |
setNetworkError(true); | |
createToast({ body: error.message }); | |
} finally { | |
setIsLoading(false); | |
} | |
}; | |
fetchMonitors(); | |
return () => controller.abort(); | |
}, [authToken, monitorId, isCreate]); |
<Dialog | ||
title="Do you want to delete this status page?" | ||
onConfirm={() => { | ||
deleteMonitor(); | ||
setIsDeleteOpen(false); | ||
navigate("/distributed-uptime"); | ||
}} | ||
onCancel={() => { | ||
setIsDeleteOpen(false); | ||
}} | ||
open={isDeleteOpen} | ||
confirmationButtonLabel="Yes, delete status page" | ||
description="Once deleted, your status page cannot be retrieved." | ||
isLoading={isDeleting || isLoading} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo, check the dialog title! It's not what you think it is! 🤔
The dialog title and description refer to deleting a "status page", but this dialog is for deleting a monitor.
Here's the fix:
<Dialog
- title="Do you want to delete this status page?"
+ title="Do you want to delete this monitor?"
onConfirm={() => {
deleteMonitor();
setIsDeleteOpen(false);
navigate("/distributed-uptime");
}}
onCancel={() => {
setIsDeleteOpen(false);
}}
open={isDeleteOpen}
- confirmationButtonLabel="Yes, delete status page"
+ confirmationButtonLabel="Yes, delete monitor"
- description="Once deleted, your status page cannot be retrieved."
+ description="Once deleted, your monitor and all associated data cannot be retrieved."
isLoading={isDeleting || isLoading}
/>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
<Dialog | |
title="Do you want to delete this status page?" | |
onConfirm={() => { | |
deleteMonitor(); | |
setIsDeleteOpen(false); | |
navigate("/distributed-uptime"); | |
}} | |
onCancel={() => { | |
setIsDeleteOpen(false); | |
}} | |
open={isDeleteOpen} | |
confirmationButtonLabel="Yes, delete status page" | |
description="Once deleted, your status page cannot be retrieved." | |
isLoading={isDeleting || isLoading} | |
/> | |
<Dialog | |
title="Do you want to delete this monitor?" | |
onConfirm={() => { | |
deleteMonitor(); | |
setIsDeleteOpen(false); | |
navigate("/distributed-uptime"); | |
}} | |
onCancel={() => { | |
setIsDeleteOpen(false); | |
}} | |
open={isDeleteOpen} | |
confirmationButtonLabel="Yes, delete monitor" | |
description="Once deleted, your monitor and all associated data cannot be retrieved." | |
isLoading={isDeleting || isLoading} | |
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Client/src/Pages/DistributedUptime/Details/index.jsx (1)
124-138
: Yo, let's make this dialog description hit harder than a beat drop! 🎵The dialog implementation is solid, but we could make the description more specific about what gets deleted.
Here's a suggestion to make it clearer:
- description="Once deleted, your monitor cannot be retrieved." + description="Once deleted, your monitor and all associated checks and data cannot be retrieved."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Client/src/Pages/DistributedUptime/Details/index.jsx
(3 hunks)
🔇 Additional comments (3)
Client/src/Pages/DistributedUptime/Details/index.jsx (3)
15-16
: Yo dawg, these imports are straight fire! 🔥Clean organization of new imports for the deletion functionality. The separation between Components and Utils sections makes it easy to understand the dependencies.
Also applies to: 23-24
30-30
: Mom's spaghetti approves these state changes! 🍝Clean implementation of state management for the deletion dialog. The hooks are properly initialized with necessary dependencies.
Also applies to: 35-35, 39-39
87-92
: These controls are cleaner than my sweater! 👕Well-structured implementation of ControlsHeader with clear prop passing. The component placement makes sense in the UI flow.
This PR implements configuration page for Distributed Uptime Monitors. It also deals with deleting DU monitors and adds some helper methods for deleting asscociated records.